Try to resolve container.name from the injected agent args #1319

lujiajing1126 · 2020-11-27T10:09:09Z

Resolve #1318

Signed-off-by: Megrez Lu <[email protected]>

codecov · 2020-11-27T10:14:45Z

Codecov Report

Merging #1319 (5a14178) into master (f250749) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1319      +/-   ##
==========================================
+ Coverage   87.35%   87.41%   +0.05%     
==========================================
  Files          89       89              
  Lines        4999     5021      +22     
==========================================
+ Hits         4367     4389      +22     
  Misses        467      467              
  Partials      165      165

Impacted Files	Coverage Δ
pkg/inject/sidecar.go	`94.76% <100.00%> (+0.61%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f250749...5a14178. Read the comment docs.

jpkrohling

cc @rubenvp8510

pkg/inject/sidecar.go

jpkrohling · 2020-11-27T14:14:05Z

pkg/inject/sidecar.go

@@ -378,3 +386,12 @@ func EqualSidecar(dep, oldDep *appsv1.Deployment) bool {
 	oldDepContainer := oldDep.Spec.Template.Spec.Containers[oldDepIndex]
 	return reflect.DeepEqual(depContainer, oldDepContainer)
 }
+
+func parseAgentTags(args []string) []string {


If we follow the refactoring mentioned above, we might not need this. The reason is that we'd have a consistent result for the map, as long as the container name doesn't change. Once it changes, we want this new name to be used in the map/args.

I partially agree for this point. Suppose we have a deployment with one single container at the very beginning, of course we can readily deduce the container.name. Then we inject several sidecars base on the annotations, let's say, istio-proxy and jaeger-agent, we will finally have three containers in this deployment.

At the second time, even if we are able to filter the jaeger-agent out, we still have two containers from which we are not able to pick the correct container.name without a prior. So I think it is still necessary to parse the Tags / Args from the existing agent container.

We should really leave the container.name out if we have more than one container (not counting the Jaeger Agent one). It's certainly possible that traces are coming from both containers in that case, so, the annotation will be wrong. Reusing the container name from an older version of the pod doesn't seem right to me.

We should really leave the container.name out if we have more than one container (not counting the Jaeger Agent one). It's certainly possible that traces are coming from both containers in that case, so, the annotation will be wrong. Reusing the container name from an older version of the pod doesn't seem right to me.

I understand your argument and I think it is quite reasonable. But the deployment may still probably restart once if we inject more than one sidecar. That would cause a serious problem in a large cluster (we have ~500 deployments in our production cluster). How can we prevent from this situation?

This will happen only if you didn't have another container before, but now you have, right? The only way to prevent that, from what I can see, is to add the agent sidecar manually, so that the operator won't manage this container for you.

Specifically, I am concerned with the scenario with two operators: one is jaeger-operator, the other is istio-operator.

Emm, what about adding an annotation to specify the container-name, if the user want to set the name explicitly.

Users can do that already by adding env vars to their own container (JAEGER_TAGS). The tags we add to the sidecar container are ones that we add based on information we can infer from the env.

Signed-off-by: Megrez Lu <[email protected]>

lujiajing1126 · 2020-11-28T06:18:01Z

I try to refactor the code following the comment made by @jpkrohling

A default map of tags is created
Existing tags are parsed if jaeger-agent is already injected
The old map is overwritten by new generated default values
The tags are now sorted in alphabetic order to keep it stable

Still a question, do we need to remove container.name if it is not in the latest deployment, or just got changed?

jpkrohling · 2020-11-30T12:50:22Z

go.mod

@@ -7,7 +7,6 @@ require (
 	github.com/ghodss/yaml v1.0.1-0.20190212211648-25d852aebe32
 	github.com/go-openapi/spec v0.19.8
 	github.com/googleapis/gnostic v0.3.1
-	github.com/miekg/dns v1.1.35 // indirect


What's up with the changes to go.mod and go.sum ?

I have reverted all changes to go.mod and go.sum. But may be this is an indirect dependency, so it will be cleaned by go mod tidy?

Might be. Once this gets merged, would you like to run a go mod tidy against master and seeing if there are differences in the go.mod?

Sure. I would like to have a try.

pkg/inject/sidecar.go

pkg/inject/sidecar_test.go

Signed-off-by: Megrez Lu <[email protected]>

pkg/inject/sidecar.go

Signed-off-by: Megrez Lu <[email protected]>

jpkrohling

LGTM. @rubenvp8510, please take a look as well. Once you give your approval, I'll merge this.

jpkrohling · 2020-12-01T09:05:25Z

pkg/inject/sidecar_test.go

-	assert.Contains(t, agentTags, "container.name=only_container")
+	agentTagsMap := parseAgentTags(dep.Spec.Template.Spec.Containers[1].Args)
+	assert.Contains(t, agentTagsMap, "container.name")
+	assert.Equal(t, agentTagsMap["container.name"], "only_container")


If you are changing this PR for some other reason, you might want to remove the Contains, as the Equals assertion already covers it.

Yes, you're right. Unnecessary Contains Removed. Also fix the same issue in two other places.

Signed-off-by: Megrez Lu <[email protected]>

lujiajing1126 · 2020-12-01T13:44:10Z

Another problem may possibly come from the different logic in deployment_controller and namespace_controller, in the latter case we do not check whether the sidecar has already been injected.

But in the deployment_controller, we check that and exclude the container from the existing container list.

IMO, it will also cause unstable status.

jpkrohling · 2020-12-01T13:45:42Z

Great catch. Would you like to change this PR here to align the logic?

lujiajing1126 · 2020-12-01T13:51:39Z

Great catch. Would you like to change this PR here to align the logic?

Sure. And I prefer not to exclude the agent from the container list since we can already reach convergence even if we inject the agent twice. What's your opionion?

jpkrohling · 2020-12-01T13:53:19Z

I'm not quite sure I understand what you mean, but send the code and I'll review it.

Signed-off-by: Megrez Lu <[email protected]>

lujiajing1126 · 2020-12-01T14:19:26Z

I'm not quite sure I understand what you mean, but send the code and I'll review it.

I have aligned deployment_controller with namespace_controller

rubenvp8510 · 2020-12-01T14:57:29Z

This looks good to me. Good refactoring!

lujiajing1126 added 2 commits November 27, 2020 17:43

add testcase to confirm the issue

65a8d62

Signed-off-by: Megrez Lu <[email protected]>

find container.name in the old container args to resolve the issue

fd27c42

Signed-off-by: Megrez Lu <[email protected]>

mergify bot assigned rubenvp8510 Nov 27, 2020

mergify bot requested review from jpkrohling and rubenvp8510 November 27, 2020 10:09

jpkrohling reviewed Nov 27, 2020

View reviewed changes

refactor tag generation

4324e78

Signed-off-by: Megrez Lu <[email protected]>

jpkrohling reviewed Nov 30, 2020

View reviewed changes

lujiajing1126 added 4 commits November 30, 2020 21:45

revert back go.sum and go.mod

4aab7d1

Signed-off-by: Megrez Lu <[email protected]>

use assert.Equal instead of reflect.DeepEqual

6fbe2ec

Signed-off-by: Megrez Lu <[email protected]>

add a test to cover an unreached branch

436bcc5

Signed-off-by: Megrez Lu <[email protected]>

refactor to use getContainerName to extract container name if possible

aef839e

Signed-off-by: Megrez Lu <[email protected]>

jpkrohling reviewed Nov 30, 2020

View reviewed changes

pkg/inject/sidecar.go Show resolved Hide resolved

add tests for getContainerName

980af69

Signed-off-by: Megrez Lu <[email protected]>

jpkrohling reviewed Dec 1, 2020

View reviewed changes

lujiajing1126 added 2 commits December 1, 2020 17:22

remove redundent Contains assertion

fa80037

Signed-off-by: Megrez Lu <[email protected]>

remove redundent Contains assertion in other two places

de6d80d

Signed-off-by: Megrez Lu <[email protected]>

do not care about agent container in the controller level

5a14178

Signed-off-by: Megrez Lu <[email protected]>

rubenvp8510 approved these changes Dec 1, 2020

View reviewed changes

mergify bot merged commit 18d32a0 into jaegertracing:master Dec 1, 2020

lujiajing1126 mentioned this pull request Dec 2, 2020

Unable to lock version for indirect dependency #1326

Closed

lujiajing1126 mentioned this pull request Feb 8, 2021

Request to release a minor version for the operator #1385

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try to resolve container.name from the injected agent args #1319

Try to resolve container.name from the injected agent args #1319

lujiajing1126 commented Nov 27, 2020

codecov bot commented Nov 27, 2020 •

edited

Loading

jpkrohling left a comment

jpkrohling Nov 27, 2020

lujiajing1126 Nov 28, 2020 •

edited

Loading

jpkrohling Nov 30, 2020

lujiajing1126 Nov 30, 2020

jpkrohling Nov 30, 2020

lujiajing1126 Nov 30, 2020

jpkrohling Dec 1, 2020

lujiajing1126 commented Nov 28, 2020 •

edited

Loading

jpkrohling Nov 30, 2020

lujiajing1126 Nov 30, 2020

jpkrohling Dec 1, 2020

lujiajing1126 Dec 1, 2020

jpkrohling left a comment

jpkrohling Dec 1, 2020

lujiajing1126 Dec 1, 2020 •

edited

Loading

lujiajing1126 commented Dec 1, 2020

jpkrohling commented Dec 1, 2020

lujiajing1126 commented Dec 1, 2020

jpkrohling commented Dec 1, 2020

lujiajing1126 commented Dec 1, 2020

rubenvp8510 commented Dec 1, 2020

Try to resolve container.name from the injected agent args #1319

Try to resolve container.name from the injected agent args #1319

Conversation

lujiajing1126 commented Nov 27, 2020

codecov bot commented Nov 27, 2020 • edited Loading

Codecov Report

jpkrohling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lujiajing1126 Nov 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lujiajing1126 commented Nov 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lujiajing1126 Dec 1, 2020 • edited Loading

Choose a reason for hiding this comment

lujiajing1126 commented Dec 1, 2020

jpkrohling commented Dec 1, 2020

lujiajing1126 commented Dec 1, 2020

jpkrohling commented Dec 1, 2020

lujiajing1126 commented Dec 1, 2020

rubenvp8510 commented Dec 1, 2020

codecov bot commented Nov 27, 2020 •

edited

Loading

lujiajing1126 Nov 28, 2020 •

edited

Loading

lujiajing1126 commented Nov 28, 2020 •

edited

Loading

lujiajing1126 Dec 1, 2020 •

edited

Loading